Support filtering snapshots by IDs #299
Conversation
Our implementation of ListSnapshots currently returns all snapshots. The request parameter also provides fields to filter by snapshot ID and source volume ID, which we have been ignoring so far. Based on discussions with SIG Storage [1], it became clear that CSI drivers are expected to process present IDs in the ListSnapshots call appropriately. While the spec marks them as OPTIONAL, it turned out that the optionality refers to the CO, not the SP. (A request for clarification has been filed in [2].) As soon as any of IDs are provided, drivers are expected to take them into account. In fact, csi-snapshotter always passes a snapshot ID in and only ever takes the first snapshot item from the list of snapshots returned, which means that our current implementation is buggy for cases where the account holds more than one snapshot (at least on Kubernetes; other COs could potentially behave differently). This change fixes the issue by retrieving the snapshot by direct lookup if a snapshot ID is given. If a source volume ID is given, we do additional filtering by only returning the snapshots that were sourced by the same volume. The existing csi-sanity tests did not catch this bug. A PR to that project was submitted [3] to close the gap. Once it is merged, we should upgrade our version of the test package. [1]: https://kubernetes.slack.com/archives/C8EJ01Z46/p1587038433091900 [2]: container-storage-interface/spec#426 [3]: kubernetes-csi/csi-test#259
|
|
||
| if resp.Links == nil || resp.Links.IsLastPage() { | ||
| break | ||
| if nextToken != 0 && req.MaxEntries != 0 { |
There was a problem hiding this comment.
Why can't you have an offset coupled with the page count in this instance?
| } | ||
| var snapshots []godo.Snapshot | ||
| for { | ||
| snaps, resp, err := d.snapshots.ListVolume(ctx, listOpts) |
There was a problem hiding this comment.
I'm a little confused with this pagination section below. It seems like we paginate through every entry from the start, ignoring the offset of NextToken that is returned. Can we just determine the "pageNumber" in DO pagination terms as a calculation of the pageCount and the offset?
There was a problem hiding this comment.
I think you're right, the logic looks off. I did not pay much attention to it because existing code was just moved into an else branch but was otherwise not modified. So I believe the pagination had been suboptimal before. Great catch. 👍
If you don't mind then I'd like to address this issue in a separate PR. FWIW, the flawed pagination should not have impacted users much since csi-snapshotter always passes in a snapshot ID in the ListSnapshots request, and our handling of that parameter is only fixed by this PR. Being good CSI citizens, however, we should still fix the pagination for the case that ListSnapshots is ever consumed exhaustively either in the future or by other COs (container orchestrators) possibly today already.
There was a problem hiding this comment.
Fix submitted in #300.
This change updates csi-test to digitalocean/csi-test@master. We use our own fork which sits on top of upstream master and comes with the following customizations: - The 'should fail when the volume is missing' test is disabled because it does not work for us right now. See [1] for details and our effort to improve upstream. - It adds yet unreleased tests for our change in [2] (specifically, [3]). Going forward, we plan to return to using the upstream, released test package. [1]: kubernetes-csi/csi-test#258 [2]: #299 [3]: kubernetes-csi/csi-test@b91f254
This change updates csi-test to digitalocean/csi-test@master. We use our own fork which sits on top of upstream master and comes with the following customizations: - The 'should fail when the volume is missing' test is disabled because it does not work for us right now. See [1] for details and our effort to improve upstream. - It adds yet unreleased tests for our change in [2] (specifically, [3]). Going forward, we plan to return to using the upstream, released test package. [1]: kubernetes-csi/csi-test#258 [2]: #299 [3]: kubernetes-csi/csi-test@b91f254
Our implementation of
ListSnapshotscurrently returns all snapshots. The request parameter also provides fields to filter by snapshot ID and source volume ID, which we have been ignoring so far.Based on discussions with SIG Storage, it became clear that CSI
drivers are expected to process present IDs in the
ListSnapshotscall appropriately. While the spec marks them as OPTIONAL, it turned out that the optionality refers to the CO, not the SP. (A request for clarification has been filed.) As soon as any of IDs are provided, drivers are expected to take them into account. In fact, csi-snapshotter always passes a snapshot ID in and only ever takes the first snapshot item from the list of snapshots returned, which means that our current implementation is buggy for cases where the account holds more than one snapshot or the desired snapshot doesn't happen to be on top of the list of returned snapshots (at least on Kubernetes; other COs could potentially behave differently).This change fixes the issue by retrieving the snapshot by direct lookup if a snapshot ID is given.
If a source volume ID is given, we do additional filtering by only returning the snapshots that were sourced by the same volume.
The existing csi-sanity tests did not catch this bug. A PR to that project was submitted to close the gap. I tested it locally and verified that the test now works. Once it is merged, we should upgrade our version of the test package.